Forms: disable device visibility on fields/inputs, honor per-viewport hiding on labels#49973
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 3 files.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
Device/viewport visibility ("Hide on…") is not honored on the field wrapper
at render time — the field render pipeline bypasses core's render_block
visibility filter — so the control did nothing. Disable the 'visibility'
block support on field and input blocks on both the JS (shared/settings,
input) and PHP (register_block_type_args filter) registrations so the
non-functional option no longer appears. Labels keep visibility support.
See FORMS-694.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aa49d16 to
5d61ca2
Compare
The per-viewport "Hide on…" option is the part that isn't honored (and on a required field can't be made safe); "hide everywhere" does work for fields, but the GB control bundles both under one supports boolean, so we disable it wholesale on fields/inputs as an interim. Comment/changelog wording only — no functional change. Labels untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ktop)
Labels kept blockVisibility support but only full-hide was wired; the
per-viewport selection was ignored because the field renderer discards the
label's own rendered output (where Gutenberg would have added the
wp-block-hidden-* classes). Read blockVisibility.viewport in the block->shortcode
bridge and add the same wp-block-hidden-{mobile,tablet,desktop} classes to the
label the field renderer emits; the matching media-query CSS is already
registered by core's render_block visibility filter (label keeps support).
Also apply label classes to the 'below' style-variation label for parity.
Verified on the frontend: hidden-on-mobile label gets the class and the
@media (width <= 480px){.wp-block-hidden-mobile{display:none}} rule is emitted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Nice work on this, the interim approach makes sense. I double-checked the part everyone was unsure about:
|
The standard jetpack/input was disabled, but the field-specific input blocks (input-range/slider, input-rating, input-image-option, phone-input, dropzone/file) define their own supports and don't share defaultSettings, so the control still showed on them. Add visibility:false to each (JS) and broaden the PHP filter to match all jetpack/input* plus phone-input and dropzone. Verified in-editor: every field-* and input block now hides the control; only label keeps it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LiamSarsfield
left a comment
There was a problem hiding this comment.
The input-block gap from last round. phone-input, range/rating/image, and dropzone are covered now in both JS and PHP, and the visibility key checks out against WP 7.0 core. 🚀
Some nice to haves that may be worth addressing:
Tests. Only the jetpack/input supports line is locked down today, so reverting the per-viewport label mapping, the full-hide case, or any widened input would stay green. That's the gap that needed a follow-up commit this round. A @dataProvider test on disable_field_visibility_support() across the name matrix (a field, a field-option-*, the prefix inputs, phone-input/dropzone, a non-forms block), plus a couple of Contact_Form_Plugin_Test cases for blockVisibility = false and viewport.mobile = false, covers it.
Grouped fields skip full-hide on their label. render_legend_as_label() never checks labelhiddenbyblockvisibility, so a fully-hidden label still renders on radio, checkbox-multiple, image-select, and rating, while render_label() honors it. It predates this PR, but since this is the change that makes labels honor visibility, finishing it here (or a fast-follow) would round it out. The per-viewport classes already reach the legend, so only the full-hide branch is missing.
Per team decision, the choice/option blocks (option, options, fieldset-image-options, and the deprecated field-option-radio/checkbox) are inputs in effect — they flatten through the same field-shortcode path, so visibility is inert on them. Add visibility:false (JS) and extend the PHP filter (drop the field-option-* exclusion; add the option containers). Label is now the only forms block keeping the control. Verified in-editor: every field/input/option block hides the control; only label shows it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y tests - render_legend_as_label() now respects labelhiddenbyblockvisibility, so a fully-hidden label is dropped on radio/checkbox-multiple/image-select/rating fields too (it already honored per-viewport hide via label_classes). Mirrors render_label(); addresses Liam's review. - Add a dataProvider matrix test for disable_field_visibility_support() (field, field-option, all input variants, option containers, label, non-forms block). - Add label blockVisibility tests for block_attributes_to_shortcode_attributes: full-hide sets the flag, per-viewport adds wp-block-hidden-* classes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LiamSarsfield
left a comment
There was a problem hiding this comment.
Nice work on this one. The disable approach is clean and the new test suites are a great addition.
I checked the WP 7.0 source since the feature isn't in-repo: supports.visibility = false is the correct opt-out and the per-viewport label CSS holds up, so the approach is sound. Three things worth a look:
- Existing "hide everywhere" fields reappear on the frontend. On released WP (6.9–7.0.x) the visibility render filter checks support before the full-hide branch, so once
visibility = falsea field savedmetadata.blockVisibility === falsestops hiding. Core fixed this on trunk (Trac #65389) but it's unreleased. Preserve saved full-hide for fields at render, or note it as interim behavior. - Grouped-field legend full-hide drops the accessible name.
render_legend_as_label()returns''with noaria-labelfallback on the<fieldset>, where the input path adds one at:945. Mirror it. - The render guards have no render-level test. Deleting the early-return keeps the suite green.
Minor: the label's supports.blockVisibility = true opt-in isn't a real key; supports.visibility = true is the explicit form.
…gend a11y Addresses review on #49973 (FORMS-694). - Fields: add a render_block filter (drop_field_hidden_everywhere) that drops a field whose metadata.blockVisibility === false before Contact_Form::parse() sees it. Disabling the visibility *support* stopped core's release-WP filter from honoring saved full-hide (the reorder fix, gutenberg#78780, is unreleased), so previously-hidden fields would reappear. Doing the drop ourselves is version-independent and required-safe: the field is fully removed from the form, so its required check never runs. - Grouped fields: when the legend label is fully hidden, move the label onto the <fieldset> as an aria-label so radio/checkbox-multiple/image-select/rating keep an accessible name, mirroring the single-input fallback. - Fix the label's support key (blockVisibility -> visibility; the old key was a no-op) and update the registration test. - Tests: unit + do_blocks integration coverage for the field drop; render-level coverage for the legend aria-label across both fieldset code paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ding Optional follow-up to the legend a11y fix (FORMS-694). The visibility control now survives only on the label block, so per-viewport hiding hides the <label>/<legend> (display:none on that viewport) while the input stays visible — leaving the field with no accessible name on that viewport. Route every aria-label fallback site (input, phone, textarea, select, grouped legend) through a single predicate that treats the label as hidden on either full-hide (labelhiddenbyblockvisibility) or per-viewport (a wp-block-hidden-* class on the label), and prefer the label text as the fallback name so it matches the visible label where the per-viewport label still shows (falling back to the placeholder only when there is no label). The render_label / render_legend_as_label early-returns stay full-hide-only, so per-viewport keeps the label rendered rather than removing it. This is intentionally a standalone commit: if we'd rather keep the PR minimal and treat per-viewport label hiding as visual-only for now, it can be dropped without touching the rest of the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @LiamSarsfield — all three addressed in
Separately, |
LiamSarsfield
left a comment
There was a problem hiding this comment.
LGTM! The disable-plus-drop approach is clean, and the render tests are a good addition. I ran the branch on a test site and opened an existing form whose field had a saved visibility setting; it loads clean, with no invalid-content warning now the support is gone. Happy to approve.
Some minor optional nits Claude found
-
The slider field misses the hidden-label
aria-labelfallback.render_slider_field()renders the label, then a bare<input type="range">, so a slider whose label is hidden (hide-everywhere or a per-viewport toggle) has no accessible name. Every other single input got the fallback this round; the slider got skipped. Same call as the others (is_label_hidden_by_block_visibility()+get_block_visibility_aria_label()), plus a hidden-label slider test. -
While you're in there: the fallback receives the pre-built
placeholder='…'fragment instead of the raw value, so an empty-label field can renderaria-label="placeholder='…'", and the phone country selector can emit an emptyaria-label. Pass the raw placeholder through, and skip the attribute when both label and placeholder are empty. Good moment to lift the input aria-label logic into a small helper like the legend one so every path shares it. -
Small one: the changelog entry starts with
Forms:inside the forms package, which the guidance says to drop. -
Minor consistency:
label/index.jsstill setsblockVisibility: truewhile the PHP now usesvisibility. Works today since visibility defaults on, but the keys should match.
…S key, changelog Follow-up to the review on #49973 (FORMS-694): - Slider: the bare <input type="range"> was the one single input missing the hidden-label aria-label fallback; add it. - Lift the input aria-label logic into a shared get_hidden_label_aria_label_attr() helper used by every path (input, phone, textarea, select, slider, legend) so none get skipped again. The fallback now takes the raw placeholder (not the pre-built placeholder='…' fragment), so an empty-label field no longer renders aria-label="placeholder='…'", and the attribute is omitted entirely when there is no label and no placeholder (no empty aria-label). - label/index.js: match the PHP fix — blockVisibility -> visibility support key. - Changelog: drop the redundant "Forms:" prefix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — all four fixed in
|
LiamSarsfield
left a comment
There was a problem hiding this comment.
LGTM!
One non-blocking follow-up worth logging: a file upload field whose label is hidden falls back to the generic aria-label="Select a file to upload.", so a screen reader announces two hidden-label upload fields the same way. Same fix as the slider: wire the shared helper into the dropzone button, or point aria-labelledby at the label.
Happy to see it in a separate PR though
…its label is hidden Follow-up to the review on #49973 (FORMS-694). The file field's dropzone <div role="button"> is the interactive control, but its accessible name is otherwise the generic "Select a file to upload.", so two upload fields whose labels are hidden are announced identically. Prefix the field name onto the dropzone's aria-label when the label is hidden (full or per-viewport), reusing the shared block-visibility name resolver — the same fallback the other single inputs got. Extracted into get_file_dropzone_aria_label() so it's unit-testable (the full file render short-circuits without an active Jetpack). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — done in You're right that the dropzone |
… paths - ReflectionMethod::setAccessible() is deprecated on PHP 8.5 (a no-op since 8.1), and the forms suite fails the build on deprecations, so the file dropzone test broke CI on 8.5. Guard the call with PHP_VERSION_ID < 80100, matching the existing pattern in Contact_Form_Block_Test. - Add render tests exercising the hidden-label aria-label fallback through the textarea, select and country-selector phone paths (the remaining changed lines that weren't covered), which also verifies the fix across those types. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LiamSarsfield
left a comment
There was a problem hiding this comment.
LGTM. The label-first aria-label fallback and the dropzone naming are both spot on, and dropping hide-everywhere fields before parse is the right way to keep required fields safe. Thanks for the quick turnaround on this
Proposed changes
Interim measure for FORMS-694 while we decide whether to fully support per-field device visibility.
What the GB control actually does. The block "visibility" support exposes two modes behind a single
supports.visibilityboolean: Hide everywhere (writesblockVisibility = false) and per-viewport "Hide on mobile/tablet/desktop" (writes{ viewport: … }).How they behave on our (dynamic) form blocks:
render_blockclass injection, sowp-block-hidden-*+ the media-query CSS never reach the field. Worse, it can't be made safe on a required field: both server (validate()) and client (isFormValid) validation are viewport-blind, so a required field hidden on mobile stays required but unfillable → dead form.required-safe), but the single boolean bundles it with the broken per-viewport mode and we can't split them.So this disables
visibilityon every field, input, and choice/option block (the only one that keeps it is the label — see below).Labels keep their support — and now honor per-viewport hiding too. Label hiding never touches input/validation/submission (it's pure presentation), so the viewport pitfall doesn't apply. Full-hide was already wired via
labelhiddenbyblockvisibility; this adds the per-viewport case: the block→shortcode bridge readsblockVisibility.viewportand adds the samewp-block-hidden-{mobile,tablet,desktop}classes Gutenberg would to the label the field renderer emits. The matching media-query CSS is already registered by core'srender_blockvisibility filter (the label keepsvisibilitysupport, so the filter runs on it — it just couldn't attach the class because that output is discarded). Applied across the default, outlined, animated, and below label styles.Applied consistently on both registrations:
shared/settings/index.js(all field blocks), plus per-blockvisibility: falseon the input variants (input,input-range,input-rating,input-image-option,phone-input,dropzone) and choice/option blocks (option,options,fieldset-image-options, deprecatedfield-option-*).register_block_type_argsfilter matching alljetpack/field-*,jetpack/input*,phone-input,dropzone, and the option containers, mirroring the JS.Related
Does this pull request change what data or activity we track or use?
No.
Testing instructions
<label>getswp-block-hidden-mobileand the@media (width <= 480px){…display:none}rule is emitted, so it hides on mobile. Full-hide on a label still removes it entirely.wp.blocks.hasBlockSupport('jetpack/field-name','visibility',true) === falsefor fields/input andtruefor labels/options; serverWP_Block_Type_Registryreports the same. PHPContact_Form_Block_Testpasses.Deferred (separate decision, FORMS-694)
Fully supporting per-field device visibility would require honoring viewport hiding inside the forms render pipeline and a viewport flag on the submission so required/validation can be skipped for the hidden viewport on both client and server — plus token handling. Likely not worth it; tracked in FORMS-694.